Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add Nextdoor Keyring Service #113

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

kevinchai0
Copy link

We are currently in the process of adding Nextdoor as an option to Wordpress's auto-social sharing feature. In order for this work to be completed, the first step is adding Nextdoor as a Keyring service in order to handle authentication / storing tokens.

@kevinchai0
Copy link
Author

A couple of questions Paul,

  1. Is there a way to test the refreshing on my local device? I tested the entire authorization flow but not sure how to force a refresh.
  2. I noticed that when you attempt to add multiple connections for the same service, it overrides your previous one / doesn't create a new one. For ND, we end up with multiple connections for the same account (see attached photo). Is there some check somewhere for duplicate connections that I'm missing?
image


function basic_ui_intro() {
/* translators: url */
echo '<p>' . sprintf( __( 'To use the Nextdoor service you need to be manually approved. Please reach out to get your client id and client secret.', 'keyring' )) . '</p>';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The end user doesn't need to provide client id and secret, which is stored in the WordPress server.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anybody using this service will need a client ID and secret. We should probably provide a link to a support doc or something where they can request credentials.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a full on support doc yet and I'm not sure if we will be creating one soon. Do you think leaving an email to contact would be fine?

includes/services/extended/nextdoor.php Show resolved Hide resolved
Copy link
Collaborator

@pablinos pablinos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to test this properly. Using the credentials you gave me, I get to an auth screen mentioning GoFundMe:

image

Clicking accept, makes a request that results in a 500 error. Could it be because I'm using a UK Nextdoor account?

}

function build_token_meta( $token ) {
$decoded_id_token = json_decode(base64_decode(explode('.', $token['id_token'])[1]));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty risky, but I think it's only like this while this is in development. We should probably put some safeguards around this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm using explode to parse the id_token since we only wanted to grab the payload and not the header and signature of the token.

https://stackoverflow.com/questions/31146111/how-do-i-extract-the-payload-data-of-a-decoded-jwt-web-token-using-php

this looks promising

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! It might just be a good idea to check that we have id_token and at least 2 elements after the explode etc. Although we might be able to guarantee all that before we get here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that stack overflow post doesn't work for our case. I added some safeguards for now.

@kevinchai0
Copy link
Author

Let's look into this during our meeting, it shouldn't be an issue with using a UK account right @renjunl ?

@kevinchai0
Copy link
Author

Hi @pablinos , I addressed the one comment you made and added some safeguards.

I looked into it a bit more and it looks like even after adding a name and an external ID, duplicate connections keep being made for nextdoor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants